Add method for native watcher initialization with fallback #441

0xpr03 · 2022-08-31T15:25:15Z

Ads a new method for initializing the recommended watcher and falling back to pollwatcher on failure to do so

Fixes #423

0xpr03 · 2022-08-31T15:29:15Z

This is a draft because:

I'd like to also change the recommended_watcher function to just call the new one, giving it more value, but would have to change its result value. Not sure if we can do that semver compatible (or just ignore it).
I've opted for an enum type instead of Box<dyn Watcher>, I could also imagine using that.
I've only handled the specific Function not implemented (os error 38) when running on docker in M1 MacOS #423 error. We could also fallback for any error.
Do we want to report the original error with the fallback ? Add some additional error kind "unavailable API" to at least report it?
This requires EventHandler to be declared either Copy or Clone for this fallback function. Even more of a SemVer break for recommended_watcher

Todo:

Add docs entry in the issue for using that
Change example to use the new function
allow debouncer init with fallback (maybe add an easier init)

CC @samuelcolvin as possible consumer

notify/src/lib.rs

JohnTitor · 2022-09-02T14:56:27Z

I'd like to also change the recommended_watcher function to just call the new one, giving it more value, but would have to change its result value. Not sure if we can do that semver compatible (or just ignore it).

I'd call it a breaking change if it changes behavior.

I've opted for an enum type instead of Box<dyn Watcher>, I could also imagine using that.

I think it's totally fine while we have take_boxed.

I've only handled the specific Function not implemented (os error 38) when running on docker in M1 MacOS #423 error. We could also fallback for any error.

These could come later :)

Do we want to report the original error with the fallback ? Add some additional error kind "unavailable API" to at least report it?

I don't have a strong opinion on that, maybe we could prepare an optional value?

This requires EventHandler to be declared either Copy or Clone for this fallback function. Even more of a SemVer break for recommended_watcher

Hmm, that sounds like a rather difficult issue and I cannot prepare a solution for that soon (I'm now fixing my dev env and cannot check your code deeply).

samuelcolvin

broadly seems fine to me, I'll probably stick with that I have now in watchfiles though since it's working well.

samuelcolvin · 2022-09-02T14:56:42Z

notify/src/lib.rs

+                #[allow(unused)]
+                ErrorKind::Io(io_error) => {
+                    #[cfg(target_os = "linux")]
+                    if io_error.raw_os_error() == Some(38) {


Is there a documented meaning to 38 or indeed other codes somewhere? I suspect we should fallback in more cases but I have no idea how to find out which ones should be an error and which should fallback.

Hm, here is the manpage though I prefer this list
Problem is that this does not tell us which of these could be emitted by our specific syscall.

samuelcolvin · 2022-09-02T14:58:23Z

notify/src/lib.rs

+
+impl WatcherFallback {
+    /// Returns the watcher inside
+    pub fn take_boxed(self) -> Box<dyn Watcher>{


personally I would prefer that this implemented watch. Is that very verbose/difficult?

that's a very good idea! and it would make the enum more worthwhile (because otherwise I don't actually see much of a value over directly boxing it, except for claiming the allocation-free throne, while not being useful at all)

Ok I tried this.. And it gets awkward.

We can't implement Watcher::new due to missing Copy/Clone on event_handler.

We can't implement a real kind(), as we don't have access to self. This kind of API is specifically not designed for runtime-selected watchers it seems. (V6 incoming.. ) We could still implement the functions without the trait, or leave them unimplemented!() for the new and the additional watcher-kind as in my new code.

Edit: But we can implement Deref and DerefMut, allowing direct Watcher::* access, but also kind(&self), which isn't the Trait, but at least gives the functionality.

0xpr03 · 2022-09-04T19:43:04Z

Please review again, the addition of Deref/DerefMut allows direct access to the watcher in our enum, making the whole stack allocation worthwhile, such that boxing is not required. Example

Open Questions:

Do we deprecate recommended_watcher() to reflect that it's kind of unhelpful as a function (in light of RecommendedWatcher) ?
Do we migrate all examples (except for one) and the docs to recommend using recommended_watcher_fallback over RecommendedWatcher ? Technically it is the only way which we can get to actually work on all systems (and can work around future issues). In a way comptime-selection was fun while it lasted. On the other hand a lot of people don't seem to care about the specific issue on M1 + Docker (you can also say people should not expect such weird emulator setups to work), but I don't see a drawback for people already using RecommendedWatcher to migrate to the dynamic approach.
Bikeshed: Is there a better name than recommended_watcher_fallback ? It's kinda long when for normal users I'd say it is the recommended way.
I have no good answer to the copy/clone requirement of the fallback logic. We need it for recommended_watcher_fallback, so it's an additional requirement. We may just leave it as that.

juliusl · 2022-10-12T22:20:37Z

Bikeshed: Is there a better name than recommended_watcher_fallback ? It's kinda long when for normal users I'd say it is the recommended way. @0xpr03

How about ensure_watcher? From my experience, ensure has a stronger implication that a resource will be obtained.

Anutrix · 2024-10-15T23:14:40Z

Why was this closed?

0xpr03 requested a review from JohnTitor August 31, 2022 15:30

0xpr03 force-pushed the fixup_423 branch 3 times, most recently from 4fad052 to d539e4c Compare August 31, 2022 15:59

JohnTitor reviewed Sep 2, 2022

View reviewed changes

notify/src/lib.rs Outdated Show resolved Hide resolved

samuelcolvin reviewed Sep 2, 2022

View reviewed changes

Add method for native watcher initialization with fallback

156f0b7

0xpr03 force-pushed the fixup_423 branch from d539e4c to 156f0b7 Compare September 2, 2022 19:58

use Deref for usability improvements

f752769

0xpr03 force-pushed the fixup_423 branch from 4ea48a1 to 31d9860 Compare September 2, 2022 20:42

add docs and deprecation

65be8dc

0xpr03 force-pushed the fixup_423 branch from 31d9860 to 65be8dc Compare September 4, 2022 19:42

This was referenced Sep 4, 2022

missing events while watching a file on WSL #254

Open

Deno.watchFs is not working in WSL environment denoland/deno#6966

Open

0xpr03 closed this Aug 16, 2023

0xpr03 deleted the fixup_423 branch August 16, 2023 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method for native watcher initialization with fallback #441

Add method for native watcher initialization with fallback #441

0xpr03 commented Aug 31, 2022 •

edited

Loading

0xpr03 commented Aug 31, 2022 •

edited

Loading

JohnTitor commented Sep 2, 2022 •

edited

Loading

samuelcolvin left a comment

samuelcolvin Sep 2, 2022

0xpr03 Sep 4, 2022

samuelcolvin Sep 2, 2022

0xpr03 Sep 2, 2022

0xpr03 Sep 2, 2022 •

edited

Loading

0xpr03 commented Sep 4, 2022 •

edited

Loading

juliusl commented Oct 12, 2022

Anutrix commented Oct 15, 2024

Add method for native watcher initialization with fallback #441

Add method for native watcher initialization with fallback #441

Conversation

0xpr03 commented Aug 31, 2022 • edited Loading

0xpr03 commented Aug 31, 2022 • edited Loading

JohnTitor commented Sep 2, 2022 • edited Loading

samuelcolvin left a comment

Choose a reason for hiding this comment

samuelcolvin Sep 2, 2022

Choose a reason for hiding this comment

0xpr03 Sep 4, 2022

Choose a reason for hiding this comment

samuelcolvin Sep 2, 2022

Choose a reason for hiding this comment

0xpr03 Sep 2, 2022

Choose a reason for hiding this comment

0xpr03 Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

0xpr03 commented Sep 4, 2022 • edited Loading

juliusl commented Oct 12, 2022

Anutrix commented Oct 15, 2024

0xpr03 commented Aug 31, 2022 •

edited

Loading

0xpr03 commented Aug 31, 2022 •

edited

Loading

JohnTitor commented Sep 2, 2022 •

edited

Loading

0xpr03 Sep 2, 2022 •

edited

Loading

0xpr03 commented Sep 4, 2022 •

edited

Loading